[docs] Add custom default props handler #15473

eps1lon · 2019-04-24T13:28:01Z

Allows resolving default values for props inside the function body

function Component(props) {
  const { value = 42 }  = props;
  return <div aria-valuenow={value} />
}

This will come in handy when we also resolve default values from the theme via hooks (see #15023).

TODO

Remove defaultProps for non-class components
- Grid
- GridListTile
- ~~[ ] Hidden~~ Leaving this for now. Adds quite a bit of bundle size for no benefit (currently)
Wait for fix: Add missing isReactForwardRefCall export reactjs/react-docgen#351 ~~resolution~~ release
Consider [Table] Use makeStyles over withStyles #15023 (rebase that PR once this is merged and extend the props finder).

mui-pr-bot · 2019-04-24T13:34:14Z

@material-ui/core: parsed: +0.03% , gzip: +0.52%

Details of bundle changes.

Comparing: 9634f29...1e321d2

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	+0.03% 🔺	+0.52% 🔺	311,296	311,374	84,652	85,092
@material-ui/core/Paper	-0.01%	+0.03% 🔺	67,805	67,800	20,152	20,158
@material-ui/core/Paper.esm	-0.00%	+0.02% 🔺	61,102	61,101	18,956	18,959
@material-ui/core/Popper	-0.03%	+0.17% 🔺	28,590	28,580	10,289	10,307
@material-ui/core/Textarea	0.00%	0.00%	5,513	5,513	2,379	2,379
@material-ui/core/TrapFocus	-1.01%	+0.06% 🔺	3,780	3,742	1,576	1,577
@material-ui/core/styles/createMuiTheme	0.00%	0.00%	15,958	15,958	5,781	5,781
@material-ui/core/useMediaQuery	0.00%	0.00%	2,106	2,106	974	974
@material-ui/lab	+0.07% 🔺	+0.23% 🔺	140,251	140,351	42,642	42,739
@material-ui/styles	0.00%	0.00%	51,272	51,272	15,165	15,165
@material-ui/system	0.00%	0.00%	11,765	11,765	3,924	3,924
Button	+0.06% 🔺	+0.16% 🔺	85,706	85,757	25,646	25,688
Modal	-0.19%	-0.01%	20,367	20,329	6,693	6,692
colorManipulator	0.00%	0.00%	3,904	3,904	1,543	1,543
docs.landing	+0.01% 🔺	+0.53% 🔺	51,531	51,535	11,369	11,429
docs.main	+0.04% 🔺	+0.17% 🔺	652,130	652,423	203,923	204,279
packages/material-ui/build/umd/material-ui.production.min.js	+0.02% 🔺	+0.65% 🔺	292,754	292,802	82,631	83,167

Generated by 🚫 dangerJS against 1e321d2

eps1lon · 2019-04-30T07:06:26Z

I think we should consider merging this. While the gzip increase is unfortunate we get at least a decrease in parsed size. It also helps apps using the /es build and only support evergreen browsers.

If we push more components away from withStyles like #15023 we need default values in object destructuring anyway.

joshwooding · 2019-04-30T16:43:52Z

Im happy to merge this. I’ll have a look at some code before I officially approve it.

To me one of the benefits of trimming down the library, by using functional components, means we can relax our reservations of increasing the bundle size a little.

packages/material-ui/src/CssBaseline/CssBaseline.js

joshwooding

Apart from that destructure, it all looks good to me :) Great work!

oliviertassinari · 2019-05-03T20:50:28Z

packages/material-ui/src/Grid/Grid.js

+      [classes[`wrap-xs-${String(wrap)}`]]: wrap != null,
+      [classes[`align-items-xs-${String(alignItems)}`]]: alignItems != null,
+      [classes[`align-content-xs-${String(alignContent)}`]]: alignContent != null,
+      [classes[`justify-xs-${String(justify)}`]]: justify != null,


Did you consider this strategy?

Suggested change

[classes[`justify-xs-${String(justify)}`]]: justify != null,

[classes[`justify-xs-${String(justify)}`]]: justify != 'flex-start',

oliviertassinari · 2019-05-03T21:07:28Z

Does is close #15042?

The only drawback I can think of: What about people accessing our defaultProps? We prepare them for the future. React will likely deprecate the default props: reactjs/rfcs#107. It will be an important change for the ecosystem. I imagine Facebook will provide a codemod for it.

docs/src/modules/utils/defaultPropsHandler.js

eps1lon · 2019-05-05T09:32:19Z

The only drawback I can think of: What about people accessing our defaultProps?

They were never part of our public API. We had one issue where someone was using them but it wasn't in a meaningful way. We have the theming for overriding default props.

oliviertassinari · 2019-05-08T13:34:21Z

@eps1lon What do we miss to merge the pull request?

Early commit to see bundle size impact.

Removes some default values that served no purpose

eps1lon · 2019-05-08T13:55:38Z

Was just waiting for a react-docgen release to use. Just need to resolve merge conflicts and it's good to go from my perspective.

mui-pr-bot · 2019-05-08T14:14:15Z

@material-ui/core: parsed: +0.03% , gzip: +0.63%
@material-ui/lab: parsed: +0.08% , gzip: +0.25%

Details of bundle changes.

Comparing: 0bb185c...b06c83b

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	+0.03% 🔺	+0.63% 🔺	316,694	316,779	85,960	86,501
@material-ui/core/Paper	-0.01%	+0.01% 🔺	67,864	67,859	20,163	20,165
@material-ui/core/Paper.esm	-0.00%	+0.01% 🔺	61,153	61,152	18,955	18,956
@material-ui/core/Popper	-0.03%	+0.20% 🔺	28,748	28,738	10,328	10,349
@material-ui/core/Textarea	0.00%	0.00%	5,513	5,513	2,384	2,384
@material-ui/core/TrapFocus	-1.00%	+0.32% 🔺	3,782	3,744	1,575	1,580
@material-ui/core/styles/createMuiTheme	0.00%	0.00%	15,960	15,960	5,779	5,779
@material-ui/core/useMediaQuery	0.00%	0.00%	2,106	2,106	974	974
@material-ui/lab	+0.08% 🔺	+0.25% 🔺	140,155	140,261	42,603	42,708
@material-ui/styles	0.00%	0.00%	51,344	51,344	15,182	15,182
@material-ui/system	0.00%	0.00%	14,463	14,463	4,180	4,180
Button	+0.07% 🔺	+0.16% 🔺	85,498	85,558	25,591	25,633
Modal	-0.19%	+0.01% 🔺	20,380	20,342	6,695	6,696
colorManipulator	0.00%	0.00%	3,904	3,904	1,543	1,543
docs.landing	+0.01% 🔺	+0.52% 🔺	51,183	51,187	11,280	11,339
docs.main	+0.04% 🔺	+0.18% 🔺	653,652	653,942	204,749	205,122
packages/material-ui/build/umd/material-ui.production.min.js	+0.02% 🔺	+0.57% 🔺	295,628	295,677	83,390	83,867

Generated by 🚫 dangerJS against b06c83b

NMinhNguyen · 2019-05-31T13:32:29Z

@eps1lon I haven't dug into the code too much but do you think this is something that could be contributed back to react-docgen? I'd be happy to assist with the effort as well 🙂

eps1lon · 2019-06-02T19:35:29Z

@NMinhNguyen If I remember correctly then it was pretty rough to integrate this in the base function because they had some weird fallback functionality and accounted for all sorts of component patterns. It looked like the base function needed some refactoring first to account for todays understanding what components are. Not sure I want to get involved into that.

You can copy the code from this implementation and use it in your own code base. We essentially just run two default props handlers: the default from react-docgen and a custom one that looks inside the function body:

// parse.js
import { defaultHandlers, parse as docgenParse } from 'react-docgen';
// https://github.com/eps1lon/material-ui/blob/b06c83bea600c31ee1b61b8c7676f7f6065c05b6/docs/src/modules/utils/defaultPropsHandler.js
import muiDefaultPropsHandler from './defaultPropsHandler';

const  reactAPI = docgenParse(src, resolvers, defaultHandlers.concat(muiDefaultPropsHandler));

I'd suggest you open an issue in react-docgen with a feature request and link to this example implementation.

eps1lon added the docs Improvements or additions to the documentation label Apr 24, 2019

eps1lon mentioned this pull request Apr 25, 2019

RFC: createElement changes and surrounding deprecations reactjs/rfcs#107

Open

oliviertassinari force-pushed the next branch from ad52812 to 5a768d7 Compare April 28, 2019 10:54

eps1lon force-pushed the docs/custom-default-props-resolver branch from 2a8f203 to d13ace0 Compare April 30, 2019 07:03

eps1lon marked this pull request as ready for review April 30, 2019 07:06

joshwooding reviewed Apr 30, 2019

View reviewed changes

packages/material-ui/src/CssBaseline/CssBaseline.js Outdated Show resolved Hide resolved

joshwooding approved these changes Apr 30, 2019

View reviewed changes

eps1lon force-pushed the docs/custom-default-props-resolver branch from d13ace0 to 6791d1a Compare May 2, 2019 11:27

oliviertassinari reviewed May 3, 2019

View reviewed changes

oliviertassinari force-pushed the docs/custom-default-props-resolver branch from 6791d1a to c636219 Compare May 4, 2019 17:49

oliviertassinari added the PR: good for merge label May 4, 2019

oliviertassinari reviewed May 4, 2019

View reviewed changes

docs/src/modules/utils/defaultPropsHandler.js Outdated Show resolved Hide resolved

eps1lon removed the PR: good for merge label May 5, 2019

eps1lon and others added 10 commits May 8, 2019 15:46

[docs] Add custom defaultPropsHandler

dae4fcf

WIP defaultProps removal

9bca98f

Early commit to see bundle size impact.

continue

c7d8496

and now to the tricky bit

4e0b049

[Grid] Resolve default props in destructuring

aade1ac

Removes some default values that served no purpose

[GridListTile] Disable eslint for docs

ba9b972

[CssBaseline] Use es5 syntax

edceaee

get the Grid default values back

3527b57

Upgrade react-docgen

65f54a0

Use react-docgen v5 beta

e101d5f

eps1lon force-pushed the docs/custom-default-props-resolver branch from 1e321d2 to a4b4742 Compare May 8, 2019 14:04

cleanup lockfile

b06c83b

eps1lon force-pushed the docs/custom-default-props-resolver branch from a4b4742 to b06c83b Compare May 8, 2019 14:05

eps1lon merged commit d183b44 into mui:next May 8, 2019

eps1lon changed the title ~~[docs] Add custom defaultPropsHandler~~ [docs] Add custom default props handler May 8, 2019

eps1lon deleted the docs/custom-default-props-resolver branch May 8, 2019 14:30

eps1lon mentioned this pull request May 16, 2019

[docs] custom default props resolver #15042

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[docs] Add custom default props handler #15473

[docs] Add custom default props handler #15473

eps1lon commented Apr 24, 2019 •

edited

Loading

mui-pr-bot commented Apr 24, 2019 •

edited

Loading

eps1lon commented Apr 30, 2019

joshwooding commented Apr 30, 2019

joshwooding left a comment

oliviertassinari May 3, 2019 •

edited

Loading

oliviertassinari commented May 3, 2019 •

edited

Loading

eps1lon commented May 5, 2019

oliviertassinari commented May 8, 2019

eps1lon commented May 8, 2019

mui-pr-bot commented May 8, 2019

NMinhNguyen commented May 31, 2019 •

edited

Loading

eps1lon commented Jun 2, 2019 •

edited

Loading

	[classes[`justify-xs-${String(justify)}`]]: justify != null,
	[classes[`justify-xs-${String(justify)}`]]: justify != 'flex-start',

[docs] Add custom default props handler #15473

[docs] Add custom default props handler #15473

Conversation

eps1lon commented Apr 24, 2019 • edited Loading

TODO

mui-pr-bot commented Apr 24, 2019 • edited Loading

eps1lon commented Apr 30, 2019

joshwooding commented Apr 30, 2019

joshwooding left a comment

Choose a reason for hiding this comment

oliviertassinari May 3, 2019 • edited Loading

Choose a reason for hiding this comment

oliviertassinari commented May 3, 2019 • edited Loading

eps1lon commented May 5, 2019

oliviertassinari commented May 8, 2019

eps1lon commented May 8, 2019

mui-pr-bot commented May 8, 2019

NMinhNguyen commented May 31, 2019 • edited Loading

eps1lon commented Jun 2, 2019 • edited Loading

eps1lon commented Apr 24, 2019 •

edited

Loading

mui-pr-bot commented Apr 24, 2019 •

edited

Loading

oliviertassinari May 3, 2019 •

edited

Loading

oliviertassinari commented May 3, 2019 •

edited

Loading

NMinhNguyen commented May 31, 2019 •

edited

Loading

eps1lon commented Jun 2, 2019 •

edited

Loading